fix: add type definitions, extract hook, and implement security harde…#705
fix: add type definitions, extract hook, and implement security harde…#705HardikTi13 wants to merge 10 commits intoaccordproject:mainfrom
Conversation
…ning Signed-off-by: Hardik Tiwari <tiwarihardik131105@gmail.com>
✅ Deploy Preview for ap-template-playground ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
mttrbrts
left a comment
There was a problem hiding this comment.
Please review build conflicts
There was a problem hiding this comment.
Pull request overview
This PR addresses build stability, type safety, and security concerns in the Template Playground application. The changes include adding type definitions for an external library, implementing XSS protection through HTML sanitization, preventing reverse tabnabbing attacks on external links, refactoring a React hook for better modularity, and adding DOMPurify as a dependency.
Changes:
- Added type definitions for
@accordproject/markdown-templateto resolve TypeScript errors - Implemented DOMPurify sanitization for HTML rendering to prevent XSS attacks
- Added
rel="noopener noreferrer"to external links in Footer component to prevent reverse tabnabbing - Extracted
useCodeSelectionhook from CodeSelectionMenu component to a separate hooks file - Added
dompurifyand@types/dompurifydependencies
Reviewed changes
Copilot reviewed 9 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/types/markdown-template.d.ts | Adds TypeScript type definitions for TemplateMarkTransformer class (though the signature doesn't match actual usage) |
| src/pages/MainContainer.tsx | Adds DOMPurify sanitization before rendering agreementHtml with dangerouslySetInnerHTML |
| src/hooks/useCodeSelection.tsx | Extracts code selection logic into a reusable hook (creates architectural coupling by returning JSX component) |
| src/editors/MarkdownEditor.tsx | Updates import path for useCodeSelection hook |
| src/editors/JSONEditor.tsx | Updates import path for useCodeSelection hook |
| src/editors/ConcertoEditor.tsx | Updates import path for useCodeSelection hook |
| src/components/Footer.tsx | Adds rel="noopener noreferrer" to all external links for security |
| src/components/CodeSelectionMenu.tsx | Removes useCodeSelection hook and monaco import (moved to separate hook file) |
| src/AgreementHtml.tsx | Adds DOMPurify sanitization before rendering agreementHtml with dangerouslySetInnerHTML |
| package.json | Adds dompurify and @types/dompurify dependencies |
| package-lock.json | Updates lock file with new dependencies and corrects @types/trusted-types optional flag |
| lint.txt | Build/test output file that should not be committed |
| lint-errors.txt | Build/test output file that should not be committed |
src/hooks/useCodeSelection.tsx
Outdated
| import CodeSelectionMenu from '../components/CodeSelectionMenu'; | ||
|
|
||
| export const useCodeSelection = (editorType: 'markdown' | 'concerto' | 'json') => { | ||
| const [selectedText, setSelectedText] = useState(''); | ||
| const [menuPosition, setMenuPosition] = useState<{ x: number; y: number } | null>(null); | ||
| const [showMenu, setShowMenu] = useState(false); | ||
| const enableCodeSelectionMenu = useAppStore((state) => state.aiConfig?.enableCodeSelectionMenu ?? true); | ||
|
|
||
|
|
||
| const handleSelection = (editor: monaco.editor.IStandaloneCodeEditor) => { | ||
| const selection = editor.getSelection(); | ||
| if (selection && !selection.isEmpty()) { | ||
| const selectedText = editor.getModel()?.getValueInRange(selection).trim(); | ||
| if (selectedText && selectedText.length > 0) { | ||
| const position = editor.getScrolledVisiblePosition(selection.getStartPosition()); | ||
| const editorContainer = editor.getDomNode()?.closest('.editorwrapper'); | ||
| const editorRect = editorContainer?.getBoundingClientRect(); | ||
|
|
||
| let x: number, y: number; | ||
|
|
||
| if (editorRect && position) { | ||
| x = Math.max(editorRect.left + 20, Math.min(position.left, editorRect.right - 150)); | ||
| y = Math.max(editorRect.top + 20, Math.min(position.top, editorRect.bottom - 50)); | ||
|
|
||
| x = Math.max(10, Math.min(x, window.innerWidth - 150)); | ||
| y = Math.max(10, Math.min(y, window.innerHeight - 50)); | ||
| } else if (position) { | ||
| x = Math.max(10, Math.min(position.left, window.innerWidth - 150)); | ||
| y = Math.max(10, Math.min(position.top, window.innerHeight - 50)); | ||
| } else { | ||
| return; | ||
| } | ||
|
|
||
| setSelectedText(selectedText); | ||
| setMenuPosition({ | ||
| x: x, | ||
| y: y | ||
| }); | ||
| setShowMenu(true); | ||
| } | ||
| } else { | ||
| setShowMenu(false); | ||
| } | ||
| } | ||
|
|
||
| const closeMenu = () => { | ||
| setShowMenu(false); | ||
| setSelectedText(''); | ||
| setMenuPosition(null); | ||
| }; | ||
|
|
||
| const MenuComponent = (enableCodeSelectionMenu && showMenu && selectedText && menuPosition) ? ( | ||
| <CodeSelectionMenu | ||
| selectedText={selectedText} | ||
| position={menuPosition} | ||
| onClose={closeMenu} | ||
| editorType={editorType} | ||
| /> | ||
| ) : null; |
There was a problem hiding this comment.
The new hook file imports CodeSelectionMenu component at line 4, creating a circular dependency. The hook is meant to be used by editors that need the code selection functionality, but it imports the component that should use the hook's return value. This defeats the purpose of extracting the hook for modularity. The MenuComponent should be rendered by the consuming component (MarkdownEditor, JSONEditor, ConcertoEditor) instead of being created inside the hook. The hook should only return the state and handlers, not the JSX component.
Resolved conflicts in package-lock.json and src/AgreementHtml.tsx Signed-off-by: HardikTi13 <tiwari.hardik1399@gmail.com>
d5f017f to
b2da04a
Compare
- Hook now only exports state and handlers (no JSX) - Editors import and render CodeSelectionMenu directly - Renamed useCodeSelection.tsx to useCodeSelection.ts (no JSX needed) Signed-off-by: HardikTi13 <tiwari.hardik1399@gmail.com>
I, Hardik Tiwari <tiwarihardik131105@gmail.com>, hereby add my Signed-off-by to this commit: 418bf33 Signed-off-by: Hardik Tiwari <tiwarihardik131105@gmail.com>
Type definitions should only be in devDependencies, not runtime dependencies. Signed-off-by: Hardik Tiwari <tiwarihardik131105@gmail.com>
mttrbrts
left a comment
There was a problem hiding this comment.
Please remove the lint txt files and the change to package.json.
Also, I recommend removing the typedefs for now, I've left a comment about how to do that properly which can be done in a separate PR.
There was a problem hiding this comment.
We should move typescript typedef upstream to the @accordproject/markdown-template package.
There is an example in the concerto repository for creating typedefs from jsdocs.
accordproject/markdown-transform#648
Closes #703
Summary
This PR fixes multiple critical issues affecting build stability, type safety, and application security. It also includes refactoring improvements to enhance modularity and remove lint warnings.
Changes
Build & Type Safety
src/types/markdown-template.d.tsto provide proper types for@accordproject/markdown-template, resolvingTemplateMarkTransformererrors.store.tseslint-disablecomments.as object,as unknown).Record<string, unknown>forTemplateMarkInterpreter.generate.monacoinCodeSelectionMenu.tsx) that were causing build failures.Refactoring
useCodeSelectionhooksrc/components/CodeSelectionMenu.tsx→src/hooks/useCodeSelection.tsxMarkdownEditorConcertoEditorJSONEditorto use the new hook location.
Security Hardening
dangerouslySetInnerHTML.src/pages/MainContainer.tsxsrc/AgreementHtml.tsxrel="noopener noreferrer"to external links insrc/components/Footer.tsxto prevent reverse tabnabbing.Result